Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths #104

Merged
merged 2 commits into from
Aug 26, 2016
Merged

[CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths #104

merged 2 commits into from
Aug 26, 2016

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Aug 20, 2016 •
edited
Loading

@oleg-nenashev oleg-nenashev changed the title [CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exce… [CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths Aug 20, 2016
@oleg-nenashev
Copy link
Member Author

@reviewbybees @jenkinsci/code-reviewers

@ghost
Copy link

ghost commented Aug 20, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

olivergondza
olivergondza reviewed Aug 20, 2016
View reviewed changes
src/main/java/org/jenkinsci/remoting/engine/JnlpServer3Handshake.java
EngineUtil.closeAndLogFailures(in, LOGGER, "CipherInputStream", socket.toString());
EngineUtil.closeAndLogFailures(out, LOGGER, "CipherOutputStream", socket.toString());
if (ex instanceof RuntimeException) {
throw (IOException) ex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast seems unsafe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unsafe... I've messed up something

@oleg-nenashev
Copy link
Member Author

@olivergondza fixed

jglick
jglick reviewed Aug 23, 2016
View reviewed changes
src/main/java/org/jenkinsci/remoting/engine/JnlpServer3Handshake.java
} catch (Exception ex) {
// Something went wrong. We want to gracefully close Cipher stream in the case they were open
EngineUtil.closeAndLogFailures(in, LOGGER, "CipherInputStream", socket.toString());
EngineUtil.closeAndLogFailures(out, LOGGER, "CipherOutputStream", socket.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐜 better (under Java 7) to use Throwable.addSuppressed. Or simpler still:

Channel channel = null;
CipherInputStream in = new CipherInputStream(SocketChannelStream.in(socket), channelCiphers.getDecryptCipher());
try {
    CipherOutputStream out = new CipherOutputStream(SocketChannelStream.out(socket), channelCiphers.getEncryptCipher());
    try {
        return channel = createChannelBuilder(nodeName).build(in, out);
    } finally {
        if (channel == null) {
            out.close();
        }
    }
} finally {
    if (channel == null) {
        in.close();
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better would be a utility (should I file a suggestion for Guava?):

public static class ClosePartials implements AutoCloseable {
    private final List<AutoCloseable> things = new ArrayList<>();
    public ClosePartials() {}
    public <T extends AutoCloseable> T with(T thing) {
        things.add(thing);
        return thing;
    }
    /** @param <T> recommended to also be {@link AutoCloseable} */
    public <T> T done(T thing) {
        things.clear();
        return thing;
    }
    @Override
    public void close() throws Exception {
        Exception x = null;
        for (AutoCloseable thing : things) {
            try {
                thing.close();
            } catch (Throwable t) {
                if (x == null) {
                    if (t instanceof Exception) {
                        x = (Exception) t;
                    } else {
                        x = new Exception(t);
                    }
                } else {
                    x.addSuppressed(t);
                }
            }
        }
        if (x != null) {
            throw x;
        }
    }
}

Example usage:

public static Channel example(Socket s, Cipher c, ChannelBuilder cb) throws Exception {
    try (ClosePartials cp = new ClosePartials();
         InputStream sin = cp.with(SocketChannelStream.in(s));
         CipherInputStream cin = cp.with(new CipherInputStream(sin, c));
         OutputStream sout = cp.with(SocketChannelStream.out(s));
         CipherOutputStream cout = cp.with(new CipherOutputStream(sout, c))) {
        return cp.done(cb.build(cin, cout));
    }
}

In general, methods which deliberately create closeable [sic] objects but then conceal them as references inside some higher-level closeable object are risky, because even if the ultimate caller is careful to close the final object, its close method might not be foolproof. Channel.close does not even pretend to guarantee that all streams it was originally passed will be closed by the time the method completes.

@jglick
Copy link
Member

jglick commented Aug 23, 2016

Anyway, 🐝 as it stands.

@oleg-nenashev
Copy link
Member Author

All these PRs go against stable-2.x, which will stay on Java6

@oleg-nenashev
Copy link
Member Author

@reviewbybees done

@oleg-nenashev oleg-nenashev merged commit 4f01088 into jenkinsci:stable-2.x Aug 26, 2016
oleg-nenashev added a commit that referenced this pull request Sep 26, 2016
@oleg-nenashev
Merge stable-2.x into master (#107)
190653b 
* [maven-release-plugin] prepare release remoting-2.62

* [maven-release-plugin] prepare for next development iteration

* [FIXED JENKINS-37539] - Prevent NPE in Engine#connect() when host or port are null or empty (#101)

Bug diagnosis:
* https://scan8.coverity.com/reports.htm#v14462/p10499/fileInstanceId=12836372&defectInstanceId=4427367&mergedDefectId=152195
* https://scan8.coverity.com/reports.htm#v14462/p10499/fileInstanceId=12836372&defectInstanceId=4427365&mergedDefectId=152194

* [CID-152201] - Fix resource leak in remoting.jnlp.Main (#102)

* [CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths (#104)

* [CID-152200,CID-152202] - Resource leak in Cipher I/O streams on exceptional paths

* https://scan8.coverity.com/reports.htm#v14462/p10499/fileInstanceId=12836377&defectInstanceId=4427377&mergedDefectId=152202
* https://scan8.coverity.com/reports.htm#v14462/p10499/fileInstanceId=12836377&defectInstanceId=4427377&mergedDefectId=152200

* [CID-152202] - Fix typo noticed by @olivergondza

* Fix another merge conflict

* Remove EngineUtil#closeAndLogFailures()
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Oct 9, 2016
@oleg-nenashev
[JENKINS-38539, JENKINS-37539] Update remoting from 2.62 to 2.62.2
3966b11 
2.61 does not exist, there was an issue during the release
Changes in 2.62: https://github.com/jenkinsci/remoting/blob/stable-2.x/CHANGELOG.md#2622

* [JENKINS-38539](https://issues.jenkins-ci.org/browse/JENKINS-38539) - 
Stability: Turn on SO_KEEPALIVE and provide CLI option to turn it off again.
(jenkinsci/remoting#110)
* [JENKINS-37539](https://issues.jenkins-ci.org/browse/JENKINS-37539) - 
Prevent <code>NullPointerException</code> in <code>Engine#connect()</code> when host or port parameters are <code>null</code> or empty.
(jenkinsci/remoting#101)
* [CID-152201] - 
Fix resource leak in <code>remoting.jnlp.Main</code>.
(jenkinsci/remoting#102)
* [CID-152200,CID-152202] - 
Resource leak in Encryption Cipher I/O streams on exceptional paths.
(jenkinsci/remoting#104)
@oleg-nenashev oleg-nenashev mentioned this pull request Oct 9, 2016
Merged
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Oct 11, 2016
@oleg-nenashev
[JENKINS-38539, JENKINS-37539] Update remoting from 2.62 to 2.62.2 (#…
7e0db20 
…2585)

2.61 does not exist, there was an issue during the release
Changes in 2.62: https://github.com/jenkinsci/remoting/blob/stable-2.x/CHANGELOG.md#2622

* [JENKINS-38539](https://issues.jenkins-ci.org/browse/JENKINS-38539) - 
Stability: Turn on SO_KEEPALIVE and provide CLI option to turn it off again.
(jenkinsci/remoting#110)
* [JENKINS-37539](https://issues.jenkins-ci.org/browse/JENKINS-37539) - 
Prevent <code>NullPointerException</code> in <code>Engine#connect()</code> when host or port parameters are <code>null</code> or empty.
(jenkinsci/remoting#101)
* [CID-152201] - 
Fix resource leak in <code>remoting.jnlp.Main</code>.
(jenkinsci/remoting#102)
* [CID-152200,CID-152202] - 
Resource leak in Encryption Cipher I/O streams on exceptional paths.
(jenkinsci/remoting#104)
oleg-nenashev added a commit to jenkinsci/jenkins that referenced this pull request Nov 6, 2016
@oleg-nenashev
Update remoting to 2.62.2
62a5ddd 
This PR picks the latest available version of remoting stable-2.x. All the fixes have been integrated into remoting-3.0 and soaked enough.

* [JENKINS-38539](https://issues.jenkins-ci.org/browse/JENKINS-38539) - 
Stability: Turn on SO_KEEPALIVE by default and provide CLI option to turn it off again.
(jenkinsci/remoting#110)
* [JENKINS-37539](https://issues.jenkins-ci.org/browse/JENKINS-37539) - 
Prevent <code>NullPointerException</code> in <code>Engine#connect()</code> when host or port parameters are <code>null</code> or empty.
(jenkinsci/remoting#101)
* [CID-152201] - 
Fix resource leak in <code>remoting.jnlp.Main</code>.
(jenkinsci/remoting#102)
* [CID-152200,CID-152202] - 
Resource leak in Encryption Cipher I/O streams on exceptional paths.
(jenkinsci/remoting#104)
@oleg-nenashev oleg-nenashev mentioned this pull request Nov 6, 2016
Merged
@jglick jglick mentioned this pull request Aug 2, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oleg-nenashev @jglick @olivergondza